Skip to content

Refactors rate_limit plugin with YAML configurations #10559

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
zwoop merged 10 commits into from
Oct 25, 2023
Merged

Refactors rate_limit plugin with YAML configurations #10559

zwoop merged 10 commits into from
Oct 25, 2023

Conversation

@zwoop
Copy link
Contributor

@zwoop zwoop commented Oct 4, 2023 •
edited
Loading

Besides majorly refactoring a lot of the global plugin code, this PR makes the following changes:

  • YAML configuration
  • Reloadable configurations (even as a global)
  • SNI aliases (one rate limiter can apply to many SNIs)
  • The IP reputation objects are now shareable for many SNIs. This means an attacker can circumvent the reputation by spreading the DDoS traffic across many SNIs.

@zwoop zwoop added the rate_limit rate_limit plugin label Oct 4, 2023
@zwoop zwoop added this to the 10.0.0 milestone Oct 4, 2023
@zwoop zwoop marked this pull request as draft October 4, 2023 17:16
@shukitchan
Copy link
Contributor

shukitchan commented Oct 4, 2023

I can review this when it is ready

@zwoop zwoop force-pushed the NewRateLimitConfigs branch 4 times, most recently from 08a267d to 5a306fb Compare October 5, 2023 00:16
@zwoop
Copy link
Contributor Author

zwoop commented Oct 5, 2023

I can review this when it is ready

Great! Almost there :). The refactoring focuses entirely on the global plugin only, leaving the remap plugin as-is. One hope with this refactoring and change to a reloadable YAML configuration is that we can add more features and rate limiting rules more easily.

@zwoop zwoop force-pushed the NewRateLimitConfigs branch 12 times, most recently from e51f9c3 to 6e41166 Compare October 5, 2023 23:20
@zwoop zwoop marked this pull request as ready for review October 5, 2023 23:57
@shukitchan shukitchan self-requested a review October 9, 2023 17:11
@zwoop zwoop force-pushed the NewRateLimitConfigs branch 2 times, most recently from eb8263b to 39f3ecd Compare October 10, 2023 17:00
@shukitchan
Copy link
Contributor

shukitchan commented Oct 10, 2023

@zwoop let me know when it is ready to review. It looks like it is but I am not sure if you are having more changes in your mind.

@bneradt
Copy link
Contributor

bneradt commented Oct 10, 2023

[approve ci autest]

brbzull0
brbzull0 reviewed Oct 13, 2023
View reviewed changes
plugins/experimental/rate_limit/sni_selector.cc Outdated

try {
config = YAML::LoadFile(yaml_file);
} catch (YAML::BadFile &e) {
Copy link
Contributor

@brbzull0 brbzull0 Oct 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are not going to change the exception right? if not then why not const.
the one below too.

Copy link
Contributor Author

@zwoop zwoop Oct 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just copied this pattern from other places in the code :). I can change it.

Copy link
Contributor

@brbzull0 brbzull0 Oct 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hope it wasn't my code! that would be a shame 🤣

@zwoop zwoop force-pushed the NewRateLimitConfigs branch from 39f3ecd to 05b5f2a Compare October 13, 2023 20:51
@zwoop zwoop force-pushed the NewRateLimitConfigs branch from 05b5f2a to 0024a9e Compare October 17, 2023 14:55
shukitchan
shukitchan previously approved these changes Oct 17, 2023
View reviewed changes
Copy link
Contributor

@shukitchan shukitchan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it looks ok.

@zwoop
Copy link
Contributor Author

zwoop commented Oct 18, 2023

[approve ci]

@zwoop zwoop force-pushed the NewRateLimitConfigs branch from 0024a9e to f6a1206 Compare October 18, 2023 23:05
shukitchan
shukitchan requested changes Oct 19, 2023
View reviewed changes
doc/admin-guide/plugins/rate_limit.en.rst Outdated
An optional metric tag to use instead of the default. When a tag is not specified
the plugin will use the FQDN of the SNI associated with each rate limiter instance
created during plugin initialization.
No queue is enable without this configuration directive, but it can also be
Copy link
Contributor

@shukitchan shukitchan Oct 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

enabled?

Copy link
Contributor Author

@zwoop zwoop Oct 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@zwoop zwoop force-pushed the NewRateLimitConfigs branch from f6a1206 to e063742 Compare October 19, 2023 15:00
@zwoop
Copy link
Contributor Author

zwoop commented Oct 19, 2023

[approve ci]

@zwoop zwoop force-pushed the NewRateLimitConfigs branch 2 times, most recently from e67c560 to bad2f5f Compare October 19, 2023 22:30
@zwoop zwoop force-pushed the NewRateLimitConfigs branch from bad2f5f to d5cabf2 Compare October 23, 2023 19:54
@zwoop
Copy link
Contributor Author

zwoop commented Oct 23, 2023

I updated this to use the updated version of the registration API.

@zwoop zwoop force-pushed the NewRateLimitConfigs branch from d5cabf2 to b842c59 Compare October 23, 2023 21:39
@zwoop zwoop merged commit 3b2e557 into apache:master Oct 25, 2023
@zwoop zwoop deleted the NewRateLimitConfigs branch October 25, 2023 20:14
@maskit maskit mentioned this pull request Aug 16, 2024
Open
91 tasks
@brbzull0 brbzull0 mentioned this pull request Aug 19, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brbzull0 brbzull0 brbzull0 left review comments

@shukitchan shukitchan shukitchan approved these changes

@bryancall bryancall Awaiting requested review from bryancall

@cmcfarlen cmcfarlen Awaiting requested review from cmcfarlen

Assignees

@zwoop zwoop

Labels

New Feature rate_limit rate_limit plugin Release Doc Done

Projects

None yet

Milestone

10.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@zwoop @shukitchan @bneradt @brbzull0